Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Concurrent source replacement can corrupt restored files, and scheduling memory and argument validation need correction.
3 open findings
What changed in this PR
Builds on #620 to improve VolumeCache hydration speed and reliability on Global Store volumes.
Changes:
- Replaces whole-file hydration with configurable parallel range reads and transient-error retries.
- Assembles files in temporary storage before atomic publication.
- Adds regression tests and documents hydration tuning.
| File | Description |
|---|---|
| tests/test_serverless/test_utils/test_rp_volume_cache.py | Tests range copying, retries, cleanup, and configuration. |
| runpod/serverless/utils/rp_volume_cache.py | Implements range-parallel hydration and shared helpers. |
| docs/serverless/volume_cache.md | Documents hydration behavior and tuning options. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Address review feedback on #621: - Pin each mirror file's size+mtime before its first range and reject the file if a range opens a replaced source, or if the mirror size disagrees with the manifest (a sync that landed after the manifest was read), instead of publishing mixed or truncated bytes. - Generate ranges lazily and pull them from hydrate_workers threads so scheduling stays O(workers) instead of one future per range. - Stop remaining ranges and remove pre-sized temp files when an unexpected (non-OSError) error aborts the hydrate. - Reject non-integer and bool tuning values at construction.
0037705 to
51ffc0a
Compare
Address review feedback on #621: - Pin each mirror file's size+mtime before its first range and reject the file if a range opens a replaced source, or if the mirror size disagrees with the manifest (a sync that landed after the manifest was read), instead of publishing mixed or truncated bytes. - Generate ranges lazily and pull them from hydrate_workers threads so scheduling stays O(workers) instead of one future per range. - Stop remaining ranges and remove pre-sized temp files when an unexpected (non-OSError) error aborts the hydrate. - Reject non-integer and bool tuning values at construction.
92fe352 to
63f9905
Compare
Address review feedback on #621: - Pin each mirror file's size+mtime before its first range and reject the file if a range opens a replaced source, or if the mirror size disagrees with the manifest (a sync that landed after the manifest was read), instead of publishing mixed or truncated bytes. - Generate ranges lazily and pull them from hydrate_workers threads so scheduling stays O(workers) instead of one future per range. - Stop remaining ranges and remove pre-sized temp files when an unexpected (non-OSError) error aborts the hydrate. - Reject non-integer and bool tuning values at construction.
63f9905 to
b2cbacb
Compare
|
|
||
| with ThreadPoolExecutor(max_workers=workers) as pool: | ||
| for future in [pool.submit(drain) for _ in range(workers)]: | ||
| future.result() |
There was a problem hiding this comment.
if hydrate gets interrupted here (say Ctrl-C), will the threads keep copying every remaining chunk before it stops? stop only gets set when a thread hits an error. maybe call stop.set() when this loop raises?
| except OSError as exc: | ||
| if exc.errno not in _RANGE_RETRY_ERRNOS: | ||
| raise | ||
| time.sleep(_RANGE_RETRY_BASE_SECONDS * 2**attempt) |
There was a problem hiding this comment.
should we add a bit of randomness to the wait? when the mount gets overloaded, all 64 threads fail together and would retry at the same moment, which could overload it again
| self._max_workers = _positive_or_default( | ||
| "max_workers", max_workers, min(32, (os.cpu_count() or 4) * 4) | ||
| ) | ||
| self._hydrate_workers = _positive_or_default("hydrate_workers", hydrate_workers, _HYDRATE_WORKERS) |
There was a problem hiding this comment.
max_workers doesn't affect hydrate anymore, right? so anyone who set it low to keep hydrate light now gets 64 threads and up to 512 MiB of memory. should we mention this in the changelog, or use max_workers for hydrate when it's set and hydrate_workers isn't?
Hydrate now splits every big file into 8 MiB ranges and copies the ranges of all files from one pool of 64 threads, each range on its own fds. Object-store mounts (Global Store's geesefs) serve one sequential stream at ~140 MiB/s, and whole-file parallelism doesn't add up. Worse, parallel whole-file copies there fail with ENOMEM, and hydrate dropped those files without retry. Measured on a 4090 pod in EU-RO-1, hydrating a 15.2 GB 7B model from a global volume: before: 9 of 11 files (2 of 4 weight shards lost to ENOMEM), 24-53s after: 11 of 11 files, byte-identical, 17-22s (670-844 MiB/s) ENOMEM/EIO/EAGAIN are retried per range with backoff. Files are assembled in a temp file and renamed only when every range copied, so a failure leaves the local file untouched. A mirror shorter than its manifest entry fails instead of restoring zero padding. Tunable via hydrate_workers and hydrate_chunk_size. Sync is unchanged.
Address review feedback on #621: - Pin each mirror file's size+mtime before its first range and reject the file if a range opens a replaced source, or if the mirror size disagrees with the manifest (a sync that landed after the manifest was read), instead of publishing mixed or truncated bytes. - Generate ranges lazily and pull them from hydrate_workers threads so scheduling stays O(workers) instead of one future per range. - Stop remaining ranges and remove pre-sized temp files when an unexpected (non-OSError) error aborts the hydrate. - Reject non-integer and bool tuning values at construction.
Hoist the last attempt out of the retry loop so _with_range_retry has no implicit fall-through return (CodeQL py/mixed-returns). Adds a test pinning the attempt count when transient errors persist.
_with_range_retry only wraps _copy_range, which returns None, so stop forwarding its result (CodeQL py/procedure-return-value-used).
b2cbacb to
de1576d
Compare


Bug
Even with #620,
VolumeCache.hydrate()on a Global Store volume silently restores only part of a model. Hydrate copies whole big files in parallel withshutil.copyfile, which usessendfileon Linux. Under that load the geesefs mount fails reads withENOMEM._copy_filelogs a warning and drops the file, nothing retries it, and the worker falls back to a cold download.It's also slow when it works. The mount serves one sequential stream at ~140 MiB/s, and reading several whole files at once doesn't add up.
Measured on a 4090 pod in EU-RO-1. A 15.2 GB Qwen2.5-7B was synced to a global volume, then hydrated into an empty dir, alternating implementations, with the page cache dropped between runs:
ENOMEM), both runsdiff -r)Fix
Hydrate splits every big file into
hydrate_chunk_sizeranges (default 8 MiB). One pool ofhydrate_workersthreads (default 64) copies the ranges of all files withpread/pwrite, each range on its own descriptors. Independent range reads scale to the host NIC (~1.1 GiB/s measured), where whole-file copies don't.ENOMEM/EIO/EAGAINwith exponential backoff (8 attempts, ~6s total). Any other error, or exhausted retries, fails the file, and its remaining ranges are skipped.os.replaced into place only after every range has copied, stamped with the manifest's source mtime as in fix(serverless): VolumeCache copies files on volumes that reject utime/chmod #620. A failed file leaves any existing local copy untouched and is not counted as restored.hydrate_workersandhydrate_chunk_sizeare validated likemax_workers.workers × chunk) is the knob that matters. The default is 512 MiB, and that's also the peak hydrate memory. In earlier testbed runs, much past ~1 GiB in flight triggeredENOMEMstorms on busy hosts.sync()is unchanged. Writes to the mount scale with the number of parallel files (~400 MiB/s at 8), and range writes into one geesefs object haven't been validated.mtime_of/mtimeplumbing from_copy_parallel/_copy_file, extracted a_tmp_pathhelper (4 call sites), and added_positive_or_defaultfor constructor validation.docs/serverless/volume_cache.mddocuments the new options and the hydrate path.Tests
14 new tests in
test_rp_volume_cache.py, written first and confirmed failing:ENOMEMis retried and the file restored.make quality-checkpasses: 1492 tests, 91.7% total coverage, andrp_volume_cache.pyis at 97%. The remaining uncovered lines predate this change.